Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mypyc] Remove unused labels and gotos #15244

Merged
merged 3 commits into from
May 22, 2023

Conversation

dosisod
Copy link
Contributor

@dosisod dosisod commented May 15, 2023

This PR removes labels which are never jumped to, as well as removing gotos that jump directly to the next label (and removes the now redundant label). I had a small issue with the GetAttr op code as it would insert C code after the end of the branch which could not be detected at the IR level, as seen in mypyc/mypyc#600 (comment) .

Here are some basic stats on the file sizes before and after this PR:

Building with MYPY_OPT_LEVEL=0, MYPY_DEBUG_LEVEL=1:

Branch .so size .c size .c lines
master 43.9 MB 79.7 MB 2290675
This PR 43.6 MB (-0.6%) 78.6 MB (-1.4%) 2179967 (-4.8%)

Building with MYPY_OPT_LEVEL=3, MYPY_DEBUG_LEVEL=0:

Branch .so size .c size .c lines
master 26.5 MB 79.7 MB 2291316
This PR 26.5 MB 78.7 MB 2179701

(I don't know why the number of lines changes between optimization levels. It's only a couple hundred lines, perhaps Mypyc is emitting different code depending on optimization level?)

Unoptimized builds seem to benefit the most, while optimized builds look pretty much the same. I didn't check to see if they are exactly the same binary, but they are probably close if not the same. The biggest win is the drop in the number of lines C code being generated.

Closes mypyc/mypyc#600

This PR removes labels which are never jumped to, as well as removing gotos
that jump directly to the next label (and removes the now redundant label).
I had a small issue with the `GetAttr` op code as it would insert C code after
the end of the branch which could not be detected at the IR level, as seen in
mypyc/mypyc#600 (comment) .

Here are some basic stats on the file sizes before and after this PR:

Building with `MYPY_OPT_LEVEL=0`, `MYPY_DEBUG_LEVEL=1`:

| Branch   | `.so` size      | `.c` size       | `.c` lines      |
|----------|-----------------|-----------------|-----------------|
| `master` | 43.9 MB         | 79.7 MB         | 2290675         |
| This PR  | 43.6 MB (-0.6%) | 78.6 MB (-1.4%) | 2179967 (-4.8%) |

Building with `MYPY_OPT_LEVEL=3`, `MYPY_DEBUG_LEVEL=0`:

| Branch   | `.so` size | `.c` size | `.c` lines |
|----------|------------|-----------|------------|
| `master` | 26.5 MB    | 79.7 MB   | 2291316    |
| This PR  | 26.5 MB    | 78.7 MB   | 2179701    |

(I don't know why the number of lines changes between optimization levels.
It's only a couple hundred lines, perhaps Mypyc is emitting different code
depending on optimization level?)

Unoptimized builds seem to benefit the most, while optimized builds look
prety much the same. I didn't check to see if they are *exactly* the same
binary, but they are probably close if not the same. The biggest win is the
drop in the number of lines C code being generated.

Closes mypyc/mypyc#600
@hauntsaninja
Copy link
Collaborator

(Note the 32-bit test started failing on master and is unrelated to your PR)

@ichard26 ichard26 requested a review from JukkaL May 15, 2023 23:21
Copy link
Collaborator

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey thank you for the PR, your perseverance is appreciated :)

I'm not finished reviewing — I'd like to double check some more potentially problematic ops — but here are my initial comments. Overall, it looks good. It's impressive that this reduces the self-compile LOC by ~5%.

As usual, it's worth noting I'm a new reviewer (and not a committer!) so I may get some things wrong. Please speak up if I do!

mypyc/codegen/emitfunc.py Show resolved Hide resolved
mypyc/codegen/emitfunc.py Outdated Show resolved Hide resolved
mypyc/codegen/emitfunc.py Outdated Show resolved Hide resolved
mypyc/codegen/emitfunc.py Show resolved Hide resolved
Copy link
Collaborator

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM*. I wasn't able to break your PR with the Cast op :)

Although... Cast ops still can merge two blocks (just like GetAttr) so it could have a possibility of breaking the control flow (as the branch gotos might not be at the end). I couldn't get it to break as a failed cast seemingly never jumps to the next block (unlike GetAttr). Honestly, I'm still pretty hesitant with the correctness / robustness of this PR.

Anyway, let's wait for Jukka's review now. Thanks!

Comment on lines +127 to +129
# Find blocks that are never jumped to or are only jumped to from the
# block directly above it. This allows for more labels and gotos to be
# eliminated during code generation.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only labels are eliminated here, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code only marks the basic blocks as "referenced" if they meet the conditions above, nothing actually gets eliminated here. And yes, during codegen, labels won't be emitted for these "unreferenced" blocks, same for gotos (assuming they are jumping to the next block).

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks! These are really good code size improvements.

@JukkaL JukkaL merged commit 6c7e480 into python:master May 22, 2023
@dosisod dosisod deleted the remove-unused-labels-and-gotos branch May 24, 2023 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get rid of unnecessary gotos in branches
5 participants